Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use exit code 2 to indicate error #954

Merged
merged 5 commits into from
Jun 19, 2018

Conversation

sirreal
Copy link
Contributor

@sirreal sirreal commented Jun 18, 2018

Exit code 1 was shared to indicate both "no results" and "error." Use status code 2 to indicate errors, similar to grep's behavior.

Fixes #948

Adds integration tests for exit codes.
Adds test helper WorkDir::assert_exit_code(&self, expected_code: i32, cmd: &mut process::Command) to match arbitrary exit codes in tests.

Testing

cargo test exit_code

Manual testing

Verify that the exit code is now 2 for error:

$ cargo run -- '*' src/main.rs ; echo "Status code: $?"

    Finished dev [unoptimized + debuginfo] target(s) in 0.0 secs
     Running `target/debug/rg '*' src/main.rs`
regex parse error:
    *
    ^
error: repetition operator missing expression
Status code: 2

And remains 0 when matches are found:

$ cargo run -- 'main' src/main.rs ; echo "Status code: $?"

    Finished dev [unoptimized + debuginfo] target(s) in 0.0 secs
     Running `target/debug/rg main src/main.rs`
56:fn main() {
Status code: 0

And 1 for no matches:

cargo run -- 'this_does_not_match' src/main.rs ; echo "Status code: $?"


    Finished dev [unoptimized + debuginfo] target(s) in 0.0 secs
     Running `target/debug/rg this_does_not_match src/main.rs`
Status code: 1

Exit code 1 was shared to indicate both "no results" and "error." Use
status code 2 to indicate errors, similar to grep's behavior.
@BurntSushi
Copy link
Owner

@sirreal What do you think about adding few tests for this? We should be able to capture your manual tests in ripgrep's integration tests.

The test helpers themselves might need some additions to test this. For example, you might consider adding something similar to assert_err that lets one specifically probe the exit status code. In that code, o.status is a std::process::ExitStatus, which means it should be easy to retrieve the exit code.

@sirreal
Copy link
Contributor Author

sirreal commented Jun 18, 2018

I was wondering about testing. I'm happy to look into that, I'll investigate soon 👍

tests/tests.rs Outdated
// See: https://github.com/BurntSushi/ripgrep/issues/948
sherlock!(
feature_948_match,
".",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should match any non-empty file

tests/tests.rs Outdated
// See: https://github.com/BurntSushi/ripgrep/issues/948
sherlock!(
feature_948_no_match,
"6d28e48b5224a42b167e{10}",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Highly unlikely to match any file. 10 random hex digits repeated 10 times.

tests/tests.rs Outdated
// See: https://github.com/BurntSushi/ripgrep/issues/948
sherlock!(
feature_948_error,
"*",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Invalid regex.

@sirreal
Copy link
Contributor Author

sirreal commented Jun 18, 2018

I've added some integration tests as suggested, I'd appreciate feedback.

tests/workdir.rs Outdated
/// Runs the given command and asserts that its exit code matches expected exit code.
pub fn assert_exit_code(&self, expected_code: i32, cmd: &mut process::Command) {
let code = cmd.status()
.expect("no status")
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These expects can just become unwrap.

Copy link
Owner

@BurntSushi BurntSushi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent! This is perfect as-is, thank you so much. :-)

@BurntSushi BurntSushi merged commit ca23a17 into BurntSushi:master Jun 19, 2018
@sirreal sirreal deleted the update/exit-code-2-on-error branch June 19, 2018 12:38
@sirreal
Copy link
Contributor Author

sirreal commented Jun 19, 2018

A pleasure! 🙌

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants